-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AVM: Implement lsig size pooling #6057
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6057 +/- ##
==========================================
- Coverage 56.20% 56.20% -0.01%
==========================================
Files 494 494
Lines 69931 69945 +14
==========================================
+ Hits 39304 39311 +7
- Misses 27953 27961 +8
+ Partials 2674 2673 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I tentatively agree with you about you thinking about |
I agree with your logic and my first attempt was to do: type LogicSig struct {
_struct struct{} `codec:",omitempty,omitemptyarray"`
// Logic signed by Sig or Msig, OR hashed to be the Address of an account.
Logic []byte `codec:"l,allocbound=config.MaxLogicSigMaxSize"`
Sig crypto.Signature `codec:"sig"`
Msig crypto.MultisigSig `codec:"msig"`
// Args are not signed, but checked by Logic
- Args [][]byte `codec:"arg,allocbound=EvalMaxArgs,allocbound=config.MaxLogicSigMaxSize"
+ Args [][]byte `codec:"arg,allocbound=EvalMaxArgs,allocbound=0"`
} This makes msgp generate: func LogicSigMaxSize() (s int) {
s = 1 + 2 + msgp.BytesPrefixSize + config.MaxLogicSigMaxSize + 4 + crypto.SignatureMaxSize() + 5 + crypto.MultisigSigMaxSize() + 4
// Calculating size of slice: z.Args
- s += msgp.ArrayHeaderSize + ((EvalMaxArgs) * (msgp.BytesPrefixSize + config.MaxLogicSigMaxSize))
+ s += msgp.ArrayHeaderSize + ((EvalMaxArgs) * (msgp.BytesPrefixSize + 0))
return
} which implements the idea that the size of the actual arguments is already captured in The issue is that we are saying that the max length on an argument is diff --git a/data/transactions/msgp_gen.go b/data/transactions/msgp_gen.go
index 7cc22db08..955c1812f 100644
--- a/data/transactions/msgp_gen.go
+++ b/data/transactions/msgp_gen.go
@@ -3306,8 +3306,8 @@ func (z *LogicSig) UnmarshalMsgWithState(bts []byte, st msgp.UnmarshalState) (o
...
- if zb0007 > config.MaxLogicSigMaxSize {
- err = msgp.ErrOverflow(uint64(zb0007), uint64(config.MaxLogicSigMaxSize))
+ if zb0007 > 0 {
+ err = msgp.ErrOverflow(uint64(zb0007), uint64(0))
return
}
@@ -3395,8 +3395,8 @@ func (z *LogicSig) UnmarshalMsgWithState(bts []byte, st msgp.UnmarshalState) (o
...
- if zb0011 > config.MaxLogicSigMaxSize {
- err = msgp.ErrOverflow(uint64(zb0011), uint64(config.MaxLogicSigMaxSize))
+ if zb0011 > 0 {
+ err = msgp.ErrOverflow(uint64(zb0011), uint64(0))
return
} My second attempt was to say instead that the max len of the arg array is one max size arg by doing:
which generates which also breaks +++ b/data/transactions/msgp_gen.go
@@ -3287,8 +3287,8 @@ func (z *LogicSig) UnmarshalMsgWithState(bts []byte, st msgp.UnmarshalState) (o
...
- if zb0005 > EvalMaxArgs {
- err = msgp.ErrOverflow(uint64(zb0005), uint64(EvalMaxArgs))
+ if zb0005 > 1 {
+ err = msgp.ErrOverflow(uint64(zb0005), uint64(1))
err = msgp.WrapError(err, "struct-from-array", "Args")
return
}
@@ -3376,8 +3376,8 @@ func (z *LogicSig) UnmarshalMsgWithState(bts []byte, st msgp.UnmarshalState) (o
...
- if zb0009 > EvalMaxArgs {
- err = msgp.ErrOverflow(uint64(zb0009), uint64(EvalMaxArgs))
+ if zb0009 > 1 {
+ err = msgp.ErrOverflow(uint64(zb0009), uint64(1)) I am out of quick fixes, would you have some other idea? Or shall we just leave it very generous as it is now? |
OK, wasn't too bad, studying algorand/msgp I found an handy @@ -39,7 +39,7 @@ type LogicSig struct {
// Args are not signed, but checked by Logic
- Args [][]byte `codec:"arg,allocbound=EvalMaxArgs,allocbound=config.MaxLogicSigMaxSize"`
+ Args [][]byte `codec:"arg,allocbound=EvalMaxArgs,allocbound=config.MaxLogicSigMaxSize,maxtotalbytes=config.MaxLogicSigMaxSize"`
} I used func LogicSigMaxSize() (s int) {
s = 1 + 2 + msgp.BytesPrefixSize + config.MaxLogicSigMaxSize + 4 + crypto.SignatureMaxSize() + 5 + crypto.MultisigSigMaxSize() + 4
// Calculating size of slice: z.Args
- s += msgp.ArrayHeaderSize + ((EvalMaxArgs) * (msgp.BytesPrefixSize + config.MaxLogicSigMaxSize))
+ s += msgp.ArrayHeaderSize + config.MaxLogicSigMaxSize
return
} and leaves everything else intact. Let me know if you like this or would consider something else instead of I like it because that's very close to the max size for the args array (a bit more because the Logic cannot be zero, a bit less because we are not including the overhead from In any case in the overall |
I was thinking about another approach. How about we just keep the |
You made me realize that indeed we need to decide which is the max limit for a logicsig arg. My first reaction would be to say 4096 to match the AVM, why artificially lower it after all. The argument I see in favor of using 1000 is not to change the old value of In sum, I see an allocbound on arg size of 4096 as the most logical choice, but if maintaining the old |
Yes, we ought to allow 4096 for individual args. I suppose it can be checked in
though it's unclear why we don't check this in txn.WellFormed. It can be an unconditional check (no need to have versioning) because old transactions are implicitly limited to < 1000 bytes. I dislike that this causes a change in There is a unit test that caught this right? |
I'm really confused by that comment I mentioned. It says:
But that doesn't seem right. But, it also hints that the TxnTag should take into account 16 "normal" transactions, but doesn't bother to do so, since it's the stateproof txn that really causes the number to blow up. (I think the TxnTag is used for groups, not single transactions) |
After further discussion, we're confident that we can make changes to the max size tests so it doesn't fail so easily, without issue. I'll make those changes in a separate PR. If you address the rest in the meantime (limit arg length, maybe remove the assembly checks) I'll get the max length changes in. |
Done, and I also think that the checks in data/transactions/logic/eval.go
...
+var errLogicSigArgTooLarge = errors.New("LogicSig argument too large")
...
@@ -1305,8 +1306,15 @@ func eval(program []byte, cx *EvalContext) (pass bool, err error) {
if (cx.EvalParams.Proto == nil) || (cx.EvalParams.Proto.LogicSigVersion == 0) {
return false, errLogicSigNotSupported
}
- if cx.txn.Lsig.Args != nil && len(cx.txn.Lsig.Args) > transactions.EvalMaxArgs {
- return false, errTooManyArgs
+ if cx.txn.Lsig.Args != nil {
+ if len(cx.txn.Lsig.Args) > transactions.EvalMaxArgs {
+ return false, errTooManyArgs
+ }
+ for _, arg := range cx.txn.Lsig.Args {
+ if len(arg) > transactions.MaxLogicSigArgSize {
+ return false, errLogicSigArgTooLarge
+ }
+ }
data/transactions/logicsig.go
// EvalMaxArgs is the maximum number of arguments to an LSig
const EvalMaxArgs = 255
+// MaxLsigArgSize is the maximum size of an argument to an LSig
+// We use 4096 to match the maximum size of a TEAL value
+// (as defined in `const maxStringSize` in package logic)
+const MaxLogicSigArgSize = 4096
...
@@ -39,7 +44,7 @@ type LogicSig struct {
Msig crypto.MultisigSig `codec:"msig"`
// Args are not signed, but checked by Logic
- Args [][]byte `codec:"arg,allocbound=EvalMaxArgs,allocbound=config.MaxLogicSigMaxSize,maxtotalbytes=config.MaxLogicSigMaxSize"`
+ Args [][]byte `codec:"arg,allocbound=EvalMaxArgs,allocbound=MaxLogicSigArgSize,maxtotalbytes=config.MaxLogicSigMaxSize"`
}
Yes, this one |
I did implement them before reading your comment, I spent some time thinking about it and my best idea in the end was to use the max block length as the max for the transaction / transaction group message tag (as per below). protocol/tags.go
@@ -84,10 +84,10 @@ const StateProofSigTagMaxSize = 6378
// Matches current network.MaxMessageLength
const TopicMsgRespTagMaxSize = 6 * 1024 * 1024
-// TxnTagMaxSize is the maximum size of a TxnTag message. This is equal to SignedTxnMaxSize()
-// which is size of just a single message containing maximum Stateproof. Since Stateproof
-// transactions can't be batched we don't need to multiply by MaxTxnBatchSize.
-const TxnTagMaxSize = 4394756
+// TxnTagMaxSize is the maximum size of a TxnTag message.
+// Matches current config.MaxTxnBytesPerBlock, the maximum lenght of a block,
+// since a transaction or transaction group cannot be larger than a block.
+const TxnTagMaxSize = 5 * 1024 * 1024
node/node_test.go
@@ -805,7 +804,7 @@ func TestMaxSizesCorrect(t *testing.T) {
require.Equal(t, ppSize, protocol.ProposalPayloadTag.MaxMessageSize())
spSize := uint64(stateproof.SigFromAddrMaxSize())
require.Equal(t, spSize, protocol.StateProofSigTag.MaxMessageSize())
- txSize := uint64(transactions.SignedTxnMaxSize())
+ txSize := uint64(config.MaxTxnBytesPerBlock)
require.Equal(t, txSize, protocol.TxnTag.MaxMessageSize()) |
Ok, we merged a looser check that should allow this to pass tests. We came to pretty much the same result (5M limit) by another path. If you merge and get green checks, I'm ready to approve and I'll talk to others to get another final approval. |
This reverts commit 91d6e09 which is not needed anymore after the merge of: AVM: Derive looser, but more principled, checks of txn max size algorand#6114
cb1b901
to
39600e5
Compare
We now let goal clerk compile app program and lsig of any sizes, even it they will be rejected by the nodes.
f7e0870
to
4de5dd0
Compare
I like it, i had to make two changes to pass all tests and make all checks green. The first change was in node/node_test.go
@@ -824,10 +824,10 @@ func TestMaxSizesCorrect(t *testing.T) {
maxCombinedTxnSize := uint64(transactions.SignedTxnMaxSize())
// subtract out the two smaller signature sizes (logicsig is biggest, it can *contain* the others)
maxCombinedTxnSize -= uint64(crypto.SignatureMaxSize() + crypto.MultisigSigMaxSize())
- // the logicsig size is *also* an overestimate, because it thinks each
- // logicsig arg can be big, but really the sum of the args and the program
- // has a max size.
- maxCombinedTxnSize -= uint64(transactions.EvalMaxArgs * config.MaxLogicSigMaxSize)
+ // the logicsig size is *also* an overestimate, because it thinks that the logicsig and
+ // the logicsig args can both be up to to MaxLogicSigMaxSize, but that's the max for
+ // them combined, so it double counts and we have to subtract one.
+ maxCombinedTxnSize -= uint64(config.MaxLogicSigMaxSize) With lsig pooling the new max size for the lsig arg array is: The second change is in the integration test test/e2e-go/cli/goal/expect/tealConsensusTest.exp where I had to comment out the tests for failing to compile an overly large lsig and an overly large app program since we took out those limitations as per this comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test you deleted is fine, as I mentioned. But I'm very surprised we don't have an e2e test that confirms the that a too big logicsig is rejected as malformed.
Maybe we do have such a test, but it is being run on a single txn group, so it retains the same behaviour as before. Could you look around for it, and if found, construct one that exercises a two txn group and the different limits?
2a34ac8
to
7953862
Compare
I did not find that test so I created it here |
I think this PR is good to go now. Let me know if you prefer that I move the integration test I added to a unit test instead |
Implement pooling of logic signatures' size across a transaction group.
This PR addresses this issue by implementing pooling of logic signatures' size across a transaction group.
As discussed in the issue, given that currently it is possible to submit a group with 16 1KB lsigs, it seems logical to allow pooling of the total size across a group.
Implementation
The pooled max lsig length for the group is set at
len(group) * LogicSigMaxSize
, as done for lsig opcode budget pooling, for both node validation and goal clerk dryrun.A group is valid if the sum of all lsigs' program and args size in the group does not exceed the pooled max lsig length.
The consensus change(
EnableLogicSigSizePooling bool
) is enabled in vFuture in this PR.To make pass the TestMaxSizesCorrect, I had to change the value of TxnTagMaxSize.
Two observations on this:
Beside updating the constant I did introduce a new version of the protocol (in vFuture) but I don't fully understand the comment so not sure if I was supposed to do something else as well.
LogicSigMaxSize
calling this function:config.MaxLogicSigMaxSize
used to be 1KB and this PR changes it to 16KB.What I don't undestand is why we multiply
config.MaxLogicSigMaxSize
byEvalMaxArgs
which is 255, ending up with what looks like an exagerrated value forLogicSigMaxSize
.I did not want to touch this in case I am missing something.
Testing plan
The PR is marked as draft since it does not includes tests yet.
I've run separately integration tests testing success and failure for single transactions and groups, both for
goal clerk dryrun
and actually sending the transactions.Before integrating the tests in the codebase I'd like to get some feedback: